BUG: Index.copy() honors 'name' parameter (#14302) #14303
Conversation
| @@ -629,7 +629,7 @@ def copy(self, name=None, deep=False, dtype=None, **kwargs): | |||
| name = name or deepcopy(self.name) | |||
| else: | |||
| new_index = self._shallow_copy() | |||
| name = self.name | |||
There was a problem hiding this comment.
Although admittedly nicer to use or, this will stop people using 0 or an empty string as a name.
There was a problem hiding this comment.
That's a good point. The "deep copy" path above uses or too. Are empty strings permitted as Index names?
There was a problem hiding this comment.
Are empty strings permitted as Index names?
Technically, yes. It's safer and more idiomatic to use if name is None.
There was a problem hiding this comment.
Roger. Will fix.
Current coverage is 85.26% (diff: 88.46%)@@ master #14303 diff @@
==========================================
Files 140 140
Lines 50598 50608 +10
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 43141 43149 +8
- Misses 7457 7459 +2
Partials 0 0
|
| levels = self.levels | ||
| labels = self.labels | ||
| names = self.names | ||
| levels = levels if levels is not None else self.levels |
There was a problem hiding this comment.
nit: it's an extra line, but less code to read/write:
if levels is None:
levels = self.levels
|
@shoyer @MaximilianR Any further comments? |
jreback
added
Bug
Indexing
| idx = pd.Index([1, 2], name='MyName') | ||
| idx2 = idx.copy(name='NewName') | ||
|
|
||
| self.assertTrue(idx.equals(idx2)) |
There was a problem hiding this comment.
this should accept name or names
There was a problem hiding this comment.
Good catch. Turns out I was overwriting names!
| multi_idx = pd.Index([(1, 2), (3, 4)], names=['MyName1', 'MyName2']) | ||
| multi_idx2 = multi_idx.copy(names=['NewName1', 'NewName2']) | ||
|
|
||
| self.assertTrue(multi_idx.equals(multi_idx2)) |
There was a problem hiding this comment.
MultiIndex.copy() does not have a scalar name parameter, only the array names.
There was a problem hiding this comment.
hmm, these signature should be the same (for Index and MI). This whole name/names biz is unfortunate, but need to maintain it.
There was a problem hiding this comment.
Should we add name to MultiIndex as part of this patch then? What would the semantics mean then? We can't just duplicate the user's input n times since that would be a naming conflict.
There was a problem hiding this comment.
we do things like this (from memory), so have a look how we do that elsewhere (you can pull out to a function if you want)
if names is not None and name is not None:
raise ValueError(....)
if names is None:
names = name
| if names is not None and name is not None: | ||
| raise TypeError("Can only provide one of `names` and `name`") | ||
| if names is None: | ||
| names = name |
There was a problem hiding this comment.
Here I assume that the user has passed-in a list for name. A scalar entry for a MultiIndex doesn't make any sense.
There was a problem hiding this comment.
most/all of this can be pulled out of Base.copy and made into a private function, maybe _validate_name(name=name, names=names) (which you can then use in both .copy
names = kwargs.get('names')
if names is not None and name is not None:
raise TypeError("Can only provide one of `names` and `name`")
if deep:
from copy import deepcopy
new_index = self._shallow_copy(self._data.copy())
name = name or deepcopy(self.name)
else:
new_index = self._shallow_copy()
name = self.name
if name is not None:
names = [name]
| if names is not None and name is not None: | ||
| raise TypeError("Can only provide one of `names` and `name`") | ||
| if names is None: | ||
| names = name |
There was a problem hiding this comment.
most/all of this can be pulled out of Base.copy and made into a private function, maybe _validate_name(name=name, names=names) (which you can then use in both .copy
names = kwargs.get('names')
if names is not None and name is not None:
raise TypeError("Can only provide one of `names` and `name`")
if deep:
from copy import deepcopy
new_index = self._shallow_copy(self._data.copy())
name = name or deepcopy(self.name)
else:
new_index = self._shallow_copy()
name = self.name
if name is not None:
names = [name]
|
@jreback I refactored the validation of I'm not sure what to make of this. |
|
@chrisaycock I restarted the failed osx job. ping when all green. lgtm. |
|
thanks @chrisaycock |
git diff upstream/master | flake8 --diff